Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IE10 WebWorkers #16

Closed
wants to merge 1 commit into from
Closed

IE10 WebWorkers #16

wants to merge 1 commit into from

Conversation

Sebmaster
Copy link
Collaborator

This provides the ability to execute code as a WebWorker.

The performance of this version is abysmal though and definitly not mergeable.
Even though it is a webworker, it locks the UI thread; I'm not sure if something is absolutly wrong with my code though.

Does something in there stick out to someone/@adambom?

This starts the work for issue #12 (new issue since you can't append pull requests to existing issues).

@Sebmaster
Copy link
Collaborator Author

So, @adambom either something was up with my browser/system yesterday, or the latest code changes fixed the UI lock in IE. Either way, I think it's fine now and works in my application with Chrome 25, IE10 and FF 20.

Have a look at it please; before you merge though, I'd like to squash the commits into a single one before you merge though (or you do that while merging in the master brach itself), so we don't clutter up the commit history of master.

@Sebmaster
Copy link
Collaborator Author

Rebased on master because of the whitespace changes yesterday.

@Sebmaster
Copy link
Collaborator Author

I squashed the commit together now since the partial comments didn't make much sense anyways. If you've verified it works it's ready to merge I guess.

@adambom
Copy link
Collaborator

adambom commented Feb 28, 2013

Here's what I'd like to do. Since the next major release is going to include a pretty major refactor to the RemoteRef API, I'd like to merge this on the "new-syntax" branch.

What we should do is create a method on P, similar to _require, called options. options takes an object as an argument and sets it as this._options.

One option, which we will document, is ie10Shim. Users can set that option prior to creating any RemoteRefs. Then any time we call the RemoteRef constructor we pass options as the third argument. In the RemoteRef constructor, where you're currently doing e.code === 18 && evaljs do e.code === 18 && options.ie10shim instead. Then we can get rid of hard coding the default url to eval.js

I think we should also throw a specific error message if we get the security error, but the ie10shim option is undefined.

Does that make sense/sound good?

@Sebmaster
Copy link
Collaborator Author

Sounds good, are you going to merge new-syntax into master soon? If not, I may have to open a new pull request and rebase onto new-syntax.

@adambom
Copy link
Collaborator

adambom commented Feb 28, 2013

I'll probably tackle the merge this weekend.

@Sebmaster Sebmaster closed this Apr 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants